General commissioning dependency injection #41833
Conversation
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the General Commissioning cluster to use dependency injection, removing its reliance on global singletons. This is a significant improvement for modularity and testability. The implementation is clean and consistently applies the new pattern across the cluster's logic. I have two minor suggestions for cleanup in the test files to further improve the codebase.
src/app/clusters/general-commissioning-server/tests/TestGeneralCommissioningCluster.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/network-commissioning/tests/TestNetworkCommissioningCluster.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the GeneralCommissioningCluster to use dependency injection instead of accessing global singletons directly. The cluster now receives its dependencies through a Context struct passed to the constructor, improving testability and explicit dependency management.
Key Changes
- Introduced a
Contextstruct containing all external dependencies (CommissioningWindowManager, ConfigurationManager, DeviceControlServer, FabricTable, FailSafeContext, PlatformManager, and optionally TermsAndConditionsProvider) - Changed cluster instantiation from eager to lazy using
LazyRegisteredServerCluster - Updated all singleton accesses within the cluster to use injected dependencies from
mClusterContext
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| general-commissioning-cluster.h | Added Context struct with dependency references; updated constructor to accept Context and OptionalAttributes |
| general-commissioning-cluster.cpp | Modified all methods to use injected dependencies from mClusterContext instead of global singletons |
| CodegenIntegration.cpp | Changed from RegisteredServerCluster to LazyRegisteredServerCluster; Context is constructed during CreateRegistration |
| TestGeneralCommissioningCluster.cpp | Updated tests to construct cluster with proper Context initialization |
| TestNetworkCommissioningCluster.cpp | Added includes and minor formatting cleanup |
| thread-border-router-management-server.cpp | Added include for general-commissioning-cluster.h header |
| app_config_dependent_sources.cmake | Added CodegenIntegration.h to target sources |
src/app/clusters/network-commissioning/tests/TestNetworkCommissioningCluster.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/network-commissioning/tests/TestNetworkCommissioningCluster.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/network-commissioning/tests/TestNetworkCommissioningCluster.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/general-commissioning-server/CodegenIntegration.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/general-commissioning-server/general-commissioning-cluster.cpp
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the General Commissioning cluster to use dependency injection, decoupling it from global singletons. This is a significant improvement for testability and maintainability. The changes are well-implemented across the cluster's source, header, and test files. The core change is the introduction of a Context struct to hold dependencies, which is passed during the cluster's construction. The implementation correctly uses a LazyRegisteredServerCluster to manage the lifecycle of the cluster instance. My review found one area for improvement in the test code to reduce duplication.
src/app/clusters/general-commissioning-server/tests/TestGeneralCommissioningCluster.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/general-commissioning-server/general-commissioning-cluster.h
Show resolved
Hide resolved
...p/clusters/thread-border-router-management-server/thread-border-router-management-server.cpp
Outdated
Show resolved
Hide resolved
|
PR #41833: Size comparison from 4940afb to 4d885df Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
| ${APP_TARGET} | ||
| PRIVATE | ||
| "${CLUSTER_DIR}/CodegenIntegration.cpp" | ||
| "${CLUSTER_DIR}/CodegenIntegration.h" |
There was a problem hiding this comment.
No such file in this PR?
There was a problem hiding this comment.
This was probably missed before and things just compile because headers will work. File does exist, but I did not change it:
2 participants
Summary
Perform a global instance decoupling for the general commissioning cluster.
Picked this cluster since it has the most external dependencies, looked to see impact (flash and RAM) for a full decoupling of this.
Resource utilization
RAM: 24 bytes generally (6 references/pointers)
FLASH: O(150 bytes) for most platforms - was 100 before I added
::Destroysupport in CodegenIntegration. Expect most overhead to be because codegen integration.NM differ is somewhat confused because of preinit_array changes, however most changes seem to be to "destroy" existing now.
Testing
No functional changes and general commissioning is heavily exercised in CI. Existing CI tests are expected to check that no functional changes were made.